feat(runtime): rebuild ACP execution as plugin adapters - #5224

Review — comment only

Targeted review of the ACP rebuild. I did not read all ~2.5k lines; I focused on packages/acp-executor-plugin/src/index.ts, packages/antigravity-acp-plugin/src/index.ts, packages/runtime-host/src/server/builtin-external-agent-plugins.ts, the generic boundary changes in packages/runtime/src/plugin-executor-{service,backend}.ts, the host wiring, and the tests.

The boundary design reads well and I think it's the right shape: ACP mechanics stay in one bundle, adapters own only launch policy, and routing/lifecycle stay with #5283's generic executor path. The items below are about robustness and one class of silent failure, not about the architecture.

Issues

1. AcpExecutor marks a conversation permanently "history-only" for every execution failure, not just process loss — packages/acp-executor-plugin/src/index.ts:212 and :215

execute()'s catch calls await this.#lose(session) unconditionally, and #lose (:508) sets session.lost = true, which makes #session() throw acp_history_only for that conversationKey for the rest of the Entry generation. That's correct for genuine process loss (owner.failed, cancel timeout at :499), but it also fires for:

• the 30s INITIALIZE_TIMEOUT_MS (:59, :283) — a slow first spawn bricks the conversation;
• acp_config_unavailable / acp_config_invalid from #applyInitialConfig — a wrong model value in the Entry config permanently disables the conversation even after the config is corrected;
• a transient checkedExecutable ENOENT while the agent app is being reinstalled;
• a caller abort during #ensureInitialized — #initialize uses AbortSignal.any([signal, timeout]) (:283), so startupSignal.throwIfAborted() on a user stop during the first prompt also lands here.

The follow-up error is also misleading: it reports "history-only because its external process was lost" when no process was ever started. Suggest only calling #lose() when the connection actually failed, and for other errors clearing session.initialization/session.owner so the next prompt can retry.

2. Agent-authored tool metadata and >64 diffs throw inside context.emit, and the ACP SDK swallows the error — packages/acp-executor-plugin/src/index.ts:421-470, packages/runtime/src/plugin-executor-service.ts:507

PluginExecutorService's emit wrapper calls normalizeOutputEvent outside its try/catch (plugin-executor-service.ts:296-306), so a validation failure throws back into #acceptTool, which runs inside the ACP session/update notification handler. The SDK's dispatch catches notification-handler errors and only console.error("Error handling notification", …) — the connection stays up, so the failure is silent in the product. Two realistic triggers:

• #acceptTool passes displayName: snapshot.title and name: snapshot.name ?? … straight from the agent. normalizeOutputEvent bounds displayName at 8192 chars and name at 256 chars with no \r\n (plugin-executor-service.ts:423-426). A long agent-authored title makes tool_start throw, and because toolUseIds is only populated on a successful tool_start (plugin-executor-backend.ts #publishOutputEvent), the later tool_result is dropped too and #closeOptionalOutput never backstops it — the tool vanishes from the transcript entirely.
• projectToolResult (:781) bounds the combined diff at MAX_TOOL_RESULT_DIFF but not the path count, while isPluginToolResultContent rejects paths.length > 64 (plugin-executor-service.ts:507). One multi-file tool call (a rename across >64 files) makes the tool_result emit throw; snapshot.terminal = true is set before the emit (:457), so no later tool_call_update retries it. The backend then backstops with the synthetic "External executor ended before reporting a tool result" for a tool that actually succeeded.

Suggest clamping displayName/name/paths before crossing the boundary (or degrading >64 paths to the text summary), setting terminal only after a successful emit, and wrapping #acceptUpdate in a try/catch that logs.

3. Retained ACP processes are unbounded — packages/acp-executor-plugin/src/index.ts:149, :255, :226

#sessions only ever grows (set in #session, cleared only in dispose), and every entry keeps a live child process plus its tree. One retained process per Maka conversation key, for the lifetime of the Entry generation, with no idle eviction, cap, or LRU. A long-running Host will accumulate one Antigravity process per conversation ever started. Either bound it (evict idle sessions — the continuity marker already encodes the "history-only" consequence) or state the limit explicitly in the README so operators know.

4. No authentication handling on the execution path — packages/acp-executor-plugin/src/index.ts:579

child.stderr is drained to nowhere and initializeResponse.authMethods is ignored (the SDK exposes it at dist/schema/zod.gen.js:1134, along with an authenticate method). The setup path merged in #5164 does parse the auth line out of stderr (packages/runtime-host/src/server/acp/antigravity.ts:144-160). If the saved Antigravity login expires, the first prompt fails as acp_execution_failed / "ACP execution failed" with no re-auth affordance and nothing logged. At minimum, surface authMethods as a distinct code so the Desktop can route back into the existing setup flow.

5. A plugin-projection failure requests a Host drain — packages/runtime-host/src/server/execution-composition.ts:1962-1967

applyRuntimePolicyMutationEffects now runs builtinExternalAgentPlugins.reconcile() inside the existing context.requestDrain(); throw error; path. The setting is already committed by then, and HostPluginPlatform already records the failure and schedules its own reconcile (#recordPackageFailure / #scheduleReconcile), so draining the whole Host because a derived, replaceable package layer failed to install seems heavier than needed. Worth confirming this is intended.

Nits

• Dropped SessionUpdate kinds — #acceptUpdate (:405) handles 4 of the 14 kinds in the SDK union. config_option_update is dropped, so the in-memory session.configOptions goes stale once the agent changes a value mid-session (relevant to Set B); plan/plan_update are dropped as well. A debug log for dropped/unknown kinds would make agent behavior diagnosable.
• Diagnostics are discarded — errorCode (:879) collapses every non-AcpRuntimeError to acp_execution_failed and safeErrorMessage (:886) to 'ACP execution failed', with no cause and no log line. This makes issues 1 and 4 very hard to diagnose in the field.
• String-sniffed error channel — active.text.trimStart().startsWith('Agent execution error:') (:200) treats model-authored transcript text as a failure signal. A response that legitimately begins with that phrase fails the turn. Prefer an explicit stop reason / _meta signal if Antigravity exposes one.
• supportsAttachments is static — the initialize response already reports agentCapabilities.promptCapabilities (image/audio/embeddedContext). Deriving support from the negotiated capabilities would avoid failing a whole turn with acp_attachments_unsupported for agents that do accept images.
• Prompt flattening — promptText folds instructions, quotes, and directory references into prose prefixes inside one text block. ACP ContentBlock supports resource_link/resource/image, which would carry that structure instead of text the agent may act on. Relatedly, session/new sends mcpServers: [], so ACP agents get none of Maka's MCP servers — worth stating that explicitly.
• dispose() throwing breaks teardown — terminate() throws 'ACP process cleanup failed' if the child is still alive at the deadline, and dispose() aggregates that, so a stuck child makes the fiber's effect cleanup fail during plugin uninstall/reload. Consider logging and continuing. (terminate also has no final close/exit await, so a child exiting just past the last poll is a false positive.)
• createWholeFileDiff (:812) emits a single whole-file hunk with no \ No newline at end of file marker, so a diff whose oldText/newText lacks a trailing newline is technically malformed for strict patch consumers.
• Bundled host runtime code — package.json lists @maka/runtime as a devDependency, but index.ts value-imports terminateChildProcessTree from @maka/runtime/process-tree-terminator, so that implementation is esbuild-bundled into dist/plugin.mjs. That's consistent with the deliberate "no cross-bundle instanceof" isolation, but it means the shipped bundle carries its own copy that won't track @maka/runtime. Worth a line in the README.
• Docs self-reference — docs/antigravity-acp-plugin-rebuild.md opens with "Why PR #5224 cannot be carried forward unchanged" / "PR #5224 predates #5283", but this is PR #5224, so the doc reads as arguing against itself. Naming it "the pre-#5283 revision of this PR" would fix it.
• Non-hosted permission denial — PluginExecutorBackend.#requestPermission (plugin-executor-backend.ts:186) returns { outcome: 'cancelled' } whenever input.hostedInteraction is absent, so every ACP permission request is denied for CLI/API/scheduled execution. Safe default, but worth documenting since it silently narrows what external agents can do headlessly.

Verified / no action needed

• Recovery ordering is correct. builtin-external-agent-plugins is registered after plugin-platform in the module array (execution-composition.ts:2504, platform module ~:2494), and recoverRuntimeHostDomainModules iterates in array order, so recover()'s packageProjections()/installPackage() hit a platform where #assertReadable() passes. Good — I checked this specifically because the coordinator depends on platform recovery.
• Digest idempotency holds. extensionPackageDirectoryContentDigest uses the same sorted-path + content hash as PluginPackageStore.decodePackage, and prepareInstall copies into its own transaction directory before the staging dir is disposed, so the "no generation churn on restart" claim is sound (and builtin-external-agent-plugins.test.ts asserts a stable authorityEpoch).
• Packaging. dependencies is in WORKSPACE_RELEASE_MANIFEST_FIELDS, so @maka/acp-executor-plugin / @maka/antigravity-acp-plugin reach the packaged app through runtime-host's production closure (files: ["dist"] includes plugin.mjs).
• Adapter isolation. isolate: { acp: true } gives the acp-runtime Entry its own acp label that children inherit; ctx.provide('acp', …) is on the runtime Entry's Context and the adapter passes its own Context explicitly, so executors.register scopes to the consumer Entry (PluginExecutorService.register reads this.ctx, which Service._bind rebinds per consumer).
• Cancellation and teardown. #awaitPrompt sends session/cancel, waits for settlement, and force-terminates only on the 15s timeout; PluginExecutorService's retirement path aborts active executions and awaits settlement, and the effect cleanup disposes the provider (process trees included).
• Antigravity launch policy matches the live setup path in packages/runtime-host/src/server/acp/antigravity.ts (BROWSER=/usr/bin/true, PYTHONUNBUFFERED=1, ANTIGRAVITY_HARNESS_PATH, cwd = dirname(executable), localharness_external helper precheck).
• Generic boundary changes are additive and validated symmetrically — PluginExecutorToolResultContent is a bounded discriminated union, file_diff already exists in the canonical ToolResultEvent shape, and normalizePermissionRequest/normalizePermissionResult validate both directions with the form withdrawn on abort.
• Concurrency. One prompt per conversation is enforced by session.active (acp_busy), and session.initialization de-dupes concurrent initialization.

Test coverage vs. the PR checklist

The checklist says tests cover "lifecycle, continuity, cancellation, permission bridging, diff projection, and adapter registration". acp-executor-plugin.test.ts has 3 tests (retention, history-only, cancellation); permission bridging and file_diff are asserted only incidentally inside the first. The "Behavior and safety" claims with no test: workspace containment including symlink escape for fs/readTextFile / fs/writeTextFile, the 8 MiB file cap, the diff-size degrade path, setConfigOption validation, adapter/config validation, process-tree termination on dispose, and the durable pluginStateStore (only a fake store is exercised). Containment and the diff-degrade path are the two I'd add first, since they're the security/robustness claims.

Coordination

#5283 is the merged base and #5222/#5385/#5386 are CLI-side ACP work that doesn't touch these packages, so I don't see a conflict. The one seam worth aligning is mcpServers: [] above: #5386 adds session-scoped ACP MCP on the CLI side, and the runtime plugin currently opts out entirely. Similarly, this PR's "Set C" (agent questions, unsupported-input presentation) overlaps #5385's interaction mapping — worth a quick sync so the two don't land incompatible contracts.